Open
Bug 1280495
Opened 9 years ago
Updated 3 years ago
(coverity) uninitialized scalar variable: mailnews/imap/src/nsImapMailFolder.cpp: |rv| is not set properly (either uninitialized and some error values are lost).
Categories
(MailNews Core :: Networking: IMAP, defect)
MailNews Core
Networking: IMAP
Tracking
(Not tracked)
NEW
People
(Reporter: ishikawa, Unassigned)
References
(Blocks 1 open bug, )
Details
(Keywords: coverity, Whiteboard: CID 1137546)
Coverity found this:
9041NS_IMETHODIMP
9042nsImapMailFolder::StoreCustomKeywords(nsIMsgWindow *aMsgWindow, const nsACString& aFlagsToAdd,
9043 const nsACString& aFlagsToSubtract, nsMsgKey *aKeysToStore, uint32_t aNumKeys, nsIURI **_retval)
9044{
1. var_decl: Declaring variable rv without initializer.
9045 nsresult rv;
2. Condition WeAreOffline(), taking true branch
9046 if (WeAreOffline())
9047 {
9048 GetDatabase();
3. Condition this->mDatabase.operator bool(), taking true branch
9049 if (mDatabase)
9050 {
4. Condition keyIndex < aNumKeys, taking false branch
9051 for (uint32_t keyIndex = 0; keyIndex < aNumKeys; keyIndex++)
9052 {
9053 nsCOMPtr <nsIMsgOfflineImapOperation> op;
9054 rv = mDatabase->GetOfflineOpForKey(aKeysToStore[keyIndex], true, getter_AddRefs(op));
9055 SetFlag(nsMsgFolderFlags::OfflineEvents);
9056 if (NS_SUCCEEDED(rv) && op)
9057 {
9058 if (!aFlagsToAdd.IsEmpty())
9059 op->AddKeywordToAdd(PromiseFlatCString(aFlagsToAdd).get());
9060 if (!aFlagsToSubtract.IsEmpty())
9061 op->AddKeywordToRemove(PromiseFlatCString(aFlagsToSubtract).get());
9062 }
9063 }
9064 mDatabase->Commit(nsMsgDBCommitType::kLargeCommit); // flush offline ops
CID 1137546 (#1 of 1): Uninitialized scalar variable (UNINIT)5. uninit_use: Using uninitialized value rv.
9065 return rv;
9066 }
9067 }
Observation:
When the for-loop is not executed once, then |rv| is not set at all. (This suggests a bug of wider scope. What if for-loop is not executed at all (?).)
(I think we should put in NS_ASSERT_STATE(aNumKeys > 0) or something similar to check for the sanity of the internal state and this code.)
It also suggests that error in one early loop of for-statement is not recorded or reported at all. *if* the subsequent rv = mDatabase->GetOfflineOpForKey(aKeysToStore[keyIndex], true, getter_AddRefs(op)); succeeds, the earlier error is forgotten. This is not right, I think.
This short piece of code leaves so much for room for appropriate comment.
What if mDatabase is null on 9049?
Shouldn't we do some error processing?
Or falling through to 9068 is OK?
Hmm...
|
||
Updated•3 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•